Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
(keeping as draft for now, I'll see if I can even have mTLS) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2482 +/- ##
==========================================
- Coverage 72.71% 72.41% -0.31%
==========================================
Files 104 105 +1
Lines 10615 10700 +85
==========================================
+ Hits 7719 7748 +29
- Misses 2425 2475 +50
- Partials 471 477 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
jotak
added
the
ok-to-test
|
New images:
They will expire after two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:6d3e9d1 make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-6d3e9d1Or as a Catalog Source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-sha-6d3e9d1
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
github-actions
bot
removed
the
ok-to-test
| } | ||
|
|
||
| // `ClientServerTLS` define the TLS configuration for both client and server sides | ||
| type ClientServerTLS struct { |
There was a problem hiding this comment.
Hey @OlivierCazade - you introduced recently "ServerTLS", and now I need yet something slightly different, to handle both sides in 1 config
If you have an idea to not have yet-another-different TLS config, let me know; But I think it's hard to do without breaking changes... So I've added a bullet in https://issues.redhat.com/browse/NETOBSERV-1735 (the v1beta3 wishlist)
leandroberetta
left a comment
leandroberetta
left a comment
There was a problem hiding this comment.
First review looks good to me, just some questions.
| You can now create a `FlowCollector` resource. Refer to the [Configuration section](#configuration) of this document. A short `FlowCollector` should work, using most default values, plus with the standalone console enabled: | ||
|
|
||
| ```bash | ||
| cat <<EOF | kubectl apply -f - |
There was a problem hiding this comment.
Can we keep this yaml snippet here too? I think it's useful for a quick view.
There was a problem hiding this comment.
yes, np - I hesitated to remove it (for less duplication) - but I agree it's useful here as well
|
|
||
| // TLS or mTLS configuration when `type` is set to `Provided`. | ||
| // +optional | ||
| ProvidedCertificates *ClientServerTLS `json:"providedCertificates,omitempty"` |
There was a problem hiding this comment.
Should we add webhooks validations for the tls feature such as checking for these certs if provided is configured?.
There was a problem hiding this comment.
I think that would go against good practices, although I'm right now trying to find a reference and don't find it. Basically, since validation webhooks live in the api package, they should in theory not rely on dependencies (or be minimalist about it), because that's typically a package that others may need to pull (e.g. another project that integrates with netobserv) and having dependencies on things like k8s clients could be troublesome. So, we try to limit dependencies here. Plus, I think to remember that we don't want to do i/o during webhook calls, but not 100% sure on that.
You can note that we're already not the good guys here, because we have a dependency on internal/pkg/cluster, which itself depends on... kube clients and so on ; that's something we should refactor at some point, but currently this refactoring would be simple, because we only use that dependency to get the cluster.Info struct, not for calling functions that would run i/o. If we start adding i/o here, we won't be able to do the refactoring anymore. (It makes me think, maybe it's time to do that refactoring..)
There was a problem hiding this comment.
hey, I'm not sure if that's what you was thinking about, but I added a simple check that the Provided config is not empty (it won't check live the presence of certificates, but at least, it makes sure the configuration is consistent)
There was a problem hiding this comment.
Yes, I was thinking about that, thanks!
Fixes netobserv#2360 - Improve certificates config with: - An optional self-signed issuer - A netobserv-ca certificate, issued by either provided or self-signed |-> A new netobserv-issuer that uses this CA | |-> which issues operator webhook & metrics certs | |-> now also issues FLP service cert |-> Copied to netobserv-privileged with trust-manager Bundle - Add knob to disable cert/trust managers - With a new TLS.md doc to describe which certificates must be provided by other means, if disabled - Add NOTES.txt which is displayed after install - Some minor renaming
Also: - add tests - improve docs - add validation hook on provided
|
@leandroberetta I fixed a bug, added tests and improved the doc in my last commit |
|
|
||
| // TLS or mTLS configuration when `type` is set to `Provided`. | ||
| // +optional | ||
| ProvidedCertificates *ClientServerTLS `json:"providedCertificates,omitempty"` |
There was a problem hiding this comment.
Yes, I was thinking about that, thanks!
jotak
added
the
ok-to-test
|
New images:
They will expire after two weeks. To deploy this build: # Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:f923490 make deploy
# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-sha-f923490Or as a Catalog Source: apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: netobserv-dev
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-sha-f923490
displayName: NetObserv development catalog
publisher: Me
updateStrategy:
registryPoll:
interval: 1m |
jotak
changed the title
|
@jotak: This pull request references NETOBSERV-1101 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@jotak: This pull request references NETOBSERV-1101 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@jotak: This pull request references NETOBSERV-1101 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
1 similar comment
|
@jotak: This pull request references NETOBSERV-1101 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@jotak: This pull request references NETOBSERV-1101 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@jotak: This pull request references NETOBSERV-1101 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
3 participants
Description
Fixes #2360
This is adding a new API config:
spec.processor.service, where users can configure TLS/mTLS.| |-> which issues operator webhook & metrics certs
| |-> now also issues FLP service cert
|-> Copied to netobserv-privileged with trust-manager Bundle
provided by other means, if disabled
Dependencies
Checklist
If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.